Skip to content

Conversation

@azjezz
Copy link
Contributor

@azjezz azjezz commented Nov 26, 2022

I currently have over 18 PHP installations locally, having to swipe between them for different projects is kinda annoying, so here's my solution for the problem :)

php-discovery will attempt to find all PHP builds in the system, even if not listed in PATH, and create a struct representing each build, containing all information necessary for ext-php-rs.

note: ext-php-rs will still have to do the work to get includes on windows, while i can certainly add it as a feature in php-discovery, i feel that it doesn't belong, and that it should do only 1 job, which is discover available PHP builds.

@azjezz azjezz force-pushed the chore/discovery branch 2 times, most recently from 35c76e4 to 0187012 Compare November 26, 2022 08:08
@ptondereau
Copy link
Member

@azjezz thank you! This indeed remove a lot of code from build. I've approved the CI workflow

@azjezz
Copy link
Contributor Author

azjezz commented Nov 26, 2022

ah! forgot to define php81! -- should pass now 🤞

@azjezz azjezz force-pushed the chore/discovery branch 2 times, most recently from 489cc2c to deea2b6 Compare November 26, 2022 13:18
@ju1ius
Copy link
Contributor

ju1ius commented Nov 26, 2022

Hi @azjezz,

While I appreciate your motivation and considerable efforts to improve the current situation, having a script taking ~10sec to scan my entire /usr directory, only to find exactly the same thing that which php finds in 5ms - that is to say not the right version because I keep all my custom PHP builds under my home directory - is not something I think I'll enjoy very much.

I'd rather have a simple environment variable override like PHP_PREFIX=<prefix> where <prefix> is the --prefix option passed to ./configure (which would imply <prefix>/bin/php and <prefix>/bin/php-config on unixes).
If PHP_PREFIX is not set, then just fallback to which php-config.

But hey, that's only my personal taste. 😉

@ptondereau
Copy link
Member

Hi @azjezz,

While I appreciate your motivation and considerable efforts to improve the current situation, having a script taking ~10sec to scan my entire /usr directory, only to find exactly the same thing that which php finds in 5ms - that is to say not the right version because I keep all my custom PHP builds under my home directory - is not something I think I'll enjoy very much.

I'd rather have a simple environment variable override like PHP_PREFIX=<prefix> where <prefix> is the --prefix option passed to ./configure (which would imply <prefix>/bin/php and <prefix>/bin/php-config on unixes).

If PHP_PREFIX is not set, then just fallback to which php-config.

But hey, that's only my personal taste. 😉

Yes sometimes, I need to test with different compiled version of php (debug) and I know where to find it. In a build context, an environment variable could be more efficient.

@ju1ius
Copy link
Contributor

ju1ius commented Nov 26, 2022

having a script taking ~10sec to scan my entire /usr directory,

I stand corrected, it does perform some globbing so it's more like under a second, which is much better than I tought initially.

Still won't do anything useful in my particular case, but simplifying the build workflow is always nice ! 👍🏻

@azjezz azjezz force-pushed the chore/discovery branch 2 times, most recently from df441de to 1b3a6c3 Compare November 26, 2022 19:59
@azjezz
Copy link
Contributor Author

azjezz commented Nov 26, 2022

i have added a soft-criteria for PHP version, which can be used to select which binary to use, for example, here i can select 8.1.0-dev ( i say soft because it won't fail if no matching build is found, but rather continue to grab the first one with a matching PHP API )
image

@azjezz
Copy link
Contributor Author

azjezz commented Nov 26, 2022

( sorry about the repeated failure on windows, not testing it ^^" )

Signed-off-by: azjezz <azjezz@protonmail.com>
@azjezz
Copy link
Contributor Author

azjezz commented Nov 26, 2022

passed ✔️

Signed-off-by: azjezz <azjezz@protonmail.com>
@azjezz
Copy link
Contributor Author

azjezz commented Nov 26, 2022

^ removed unused import, won't have any effect on the build result

Copy link
Member

@ptondereau ptondereau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants